With the current URLSession testing infrastructure, a test case for this, is a challenge.

@parkera @naithar Can you please review and comment?

I have no comments on implementation, except for adding guard `self` = self else { return } inside DispatchWorkerItem closure. But i'm not really sure if it actually affects anything

As for testing, if I understand the problem correctly, TestURLSessionServer initializer could receive a delay interval for response, like init(port: UInt16, delay: TimeInterval? = nil) which could be used by .readAndRespond() method to delay actual response.
Here's the changes for TestURLSessionServer class and runServer(...) method that seem to correctly implement the timeout testing (I've changed nil default to 5 to test it): https://gist.github.com/naithar/7f26c24dd08c312858d13d4f5e97f728

Oh, now I see that i didn't understand the problem with tests correctly.

Thanks @naithar I have added the guard

@pushkarnk
Again about testing. Seems like i've started digging in a right place actually.
If we can change a func writeData(data: String) method to split resulted string in chunks and send it with a delay this could emulate the request to http://httpbin.org/drip?duration=10&numbytes=10&code=200 which is used in bug report. But currently i managed only to send multiple data parameter in a single response.

var bytes = Array(data.utf8)


for _ in 0..<5 {
    sleep(1)
    _  = try? attempt("write", valid: isNotNegative, CInt(write(connectionSocket, &bytes, bytes.count)))
}

The diff looks reasonable but we should be able to get some kind of unit test in place for this.

@naithar Thanks for your work. I will try out your suggestion. I was hoping that we can also delay the response as in:

    public func respond(with response: _HTTPResponse, delay: Int = 0) throws {
        let semaphore = DispatchSemaphore(value: 0)
        let deadlineTime = DispatchTime.now() + .seconds(delay)
        DispatchQueue.main.asyncAfter(deadline: deadlineTime) {
            do {
                try self.socket.writeData(data: response.description)
                semaphore.signal()
            } catch { }
        }
        semaphore.wait()
     }

I did this and I see a crash. Investigating it now.

Does crash message anything? Maybe "Trying to access a behaviour for a task that in not in the registry."? I can sometimes reproduce this by just delaying response for long enough.

This sometimes happens, because .taskCompleted state is set before .transferFailed, so task is no longer in registry when .completeTask(...) method is called.

Not sure what happened, but reapplying a PR to current master removed the crash.
This commit implements a chunked response, so this PR could be tested. It's ugly but it works.

@naithar Yes, I did see that fatal error. Nevertheless, your patch looks good and the test runs well. I am wondering how we go about. I'd suggest we have this PR merged first and then you create a PR with your patch.

I also realised that I need to cancel the timer when completeTask() is called.

@pushkarnk i can also create a PR with test commented out. Actually i'm not agains you taking this test implementation into this PR.

You creating the PR with calls to the new tests commented out is a better idea, in my opinion :)

Then I'll do it now.

Submitted #913

I can consistently see the "Trying to access a behaviour for a task that in not in the registry." failure with @naithar 's test case from #913 . Investigating. It is probably related to the previous test_cancelTask test and the fact that we use the same URLSession object.

That's strange.
I can see same failure. But moving test_taskTimeout test to the top of the tests array actually solves the issue.

I'll look into it a little more.

Yes, looks like we need to cancel the timeoutTimer in complete(withError:) too.

Actually test_taskTimeout causes failure only if it follows test_cancelTask test. Atm I'm not sure what exactly causing the failure.

Also moving test_cancelTask to the top causes test_dataTaskWithURL fail for the same reason as timeout test - Trying to access a behaviour for a task that in not in the registry

Running Cancel test actually causes this error for TestURLSessionServer:

bind failed: Unknown error: -1 (-1)
  - operation : "bind"
  - errno : -1
  - file : "HTTPServer.swift"
  - line : 69

It's not displayed or handled because server is starting with this code (i guess the problem is in XCTAssertTrue(true)), so it won't show anything:

globalDispatchQueue.async {
            do {
                try self.runServer(with: serverReady)
            } catch {
                XCTAssertTrue(true)
                return
            }
        }

Also not signaling semaphore in catch in runServer(...) method causes tests to hang sometimes.

@pushkarnk I see several workarounds:

• Make cancel test run last.
• Disable server start for cancel test (I guess server just hangs and waits for request, which never happen in test_cancelTask)
• Somehow change TestURLSessionServer or runServer(...) so it won't hang.

Switching

} catch let e as ServerError {
    if e.operation != "bind" { continue }

with

} catch let e as ServerError {
    if e.operation == "bind" { continue }

in runServer(...) method seems like a workaround and removes the error and crashes.

Thanks @naithar That seems like a fix (not a workaround). We retry on a bind failure and throw an error otherwise.

@swift-ci please test

@swift-ci please test and merge